GL_TEXTURE_RECTANGLE texture target support #997

JerryShih · 2017-03-21T04:05:50Z

@glennw @kvark @nical @sotaroikeda
The hardware video decoder in mac platform is usually decoding the video content into MacIOSurface. The MacIOSurface could just map to an GL_TEXTURE_RECTANGLE gl texture without any uploading. That's good for video playing.
These patches try to add a new type of external gl rect image and its corresponding shader.

This change is

JerryShih · 2017-03-21T04:13:43Z

There is still a the black screen when I pass an GL_TEXTURE_RECTANGLE gl texture to WR. I have no idea why I can't see the video content. Maybe @nical @kvark @glennw could point out the error for my patch.

bors-servo · 2017-03-21T04:20:27Z

☔ The latest upstream changes (presumably #995) made this pull request unmergeable. Please resolve the merge conflicts.

JerryShih

0ce9f97:

This commit changes the enum ImageData and SourceTexture def.
0ce9f97#diff-9aae73cdde1c142339d62fa040c56ec1R86

0ce9f97#diff-d92392172ea565b8624a34bb15391bccR52

Then we could pass the different external gl handle around the rendering path. Then, we could get the image type in SourceSurface to select the correct shader(We are trying to handle TEXTURE_2D and TEXTURE_RECT).

JerryShih · 2017-03-21T09:00:28Z

webrender/res/shared.glsl

 uniform sampler2D sColor0;
 uniform sampler2D sColor1;
 uniform sampler2D sColor2;
+#else
+uniform sampler2DRect sColor0;


macIOSurface should use sampler2DRect

JerryShih · 2017-03-21T09:01:17Z

webrender/res/ps_image_rect.fs.glsl

+
+    alpha = alpha * float(all(bvec2(step(position_in_tile, vStretchSize))));
+
+    oFragColor = vec4(alpha) * texture(sColor0, st);


textureLod doesn't support Sample2DRect. Use texture() instead.
And the range of each st components are not in [0.0, 1.0]. They are in [0, texture_size.width] and [0, texture_size.height].

JerryShih · 2017-03-21T09:07:46Z

webrender/src/renderer.rs

@@ -659,6 +662,13 @@ impl Renderer {
                                 options.precache_shaders)
        };

+        let ps_image_rect = try!{


I add a new shader for textureRect. We could also merge this back to the original image shader. I don't like too much "#ifdef" in the code.

JerryShih · 2017-03-21T09:08:52Z

webrender/src/tiling.rs

+                    SourceTexture::External(ext_image) => {
+                        match ext_image.image_type {
+                            ExternalImageType::Texture2DHandle => AlphaBatchKind::Image,
+                            ExternalImageType::TextureRectHandle => AlphaBatchKind::ImageRect,


When we have "ExternalImageType::TextureRectHandle", use ImageRect shader for this type.

nical · 2017-03-21T09:13:14Z

For reference, Gecko appears to be using texture2DRect and sampler2DRect:
https://searchfox.org/mozilla-central/rev/ee7cfd05d7d9f83b017dd54c2052a2ec19cbd48b/gfx/layers/opengl/OGLShaderProgram.cpp#368

I would prefer to not add extra shader source files for this (and just #define our way into implementing this)

kvark · 2017-03-21T19:22:35Z

I'd like to avoid first-class support for texture rectangles in WR. This concept is not supported outside of GL land, so it would make it more difficult to introduce more graphics backends. Instead, I'd like to see as minimal hack as possible to support MacIOSurface's limitations.

nical · 2017-03-21T21:03:10Z

We don't have a choice on OSX as it's the only way to render hardware accelerated video frames as far as I know.

kvark · 2017-03-21T21:06:05Z

@nical yes, that's why I said let's hack this in. Doing a compile switch for the shaders is certainly a smaller hack then exposing the rectangles as a valid texture type.

nical · 2017-03-21T22:40:32Z

Ah yes, I agree. And not such a bad hack since the only things that change are the uniform sampler definition and sampling function name. We already have compile time switches for bigger things like transforms.
Even for the rest of the code I'm sure we can simplify things a fair amount (just need to pass enough information to select the proper texture target).

JerryShih · 2017-03-22T03:08:16Z

@kvark @nical

I'd like to avoid first-class support for texture rectangles in WR. This concept is not supported outside of GL land, so it would make it more difficult to introduce more graphics backends.

Yes, this concept is not supported outside of GL land. But why this is the problem if we try to have more graphics backends? We could just have a panic hint for the new backend if we try to use the non-supported TextureRectHandle feature.

And how could we recognize the current usage for texture2d or textureRect without adding the new type of externalImage? That information is required to switch the different shaders for texture2d and textureRect. We might have both usage of texture2d or textureRect at mac platform.

So, I extend the external image type into:

+pub enum ExternalImageType {
+    Texture2DHandle,    // gl TEXTURE_2D handle
+    TextureRectHandle,  // gl TEXTURE_RECT handle
+    ExternalBuffer,
+}

0ce9f97#diff-9aae73cdde1c142339d62fa040c56ec1

Most of changes are for these new types updating.

kvark · 2017-03-22T03:40:46Z

@JerryShih yeah, having ExternalImageType::TextureRectHandle case for this seems to be unavoidable.

JerryShih · 2017-03-22T04:18:31Z

@kvark @nical

@JerryShih yeah, having ExternalImageType::TextureRectHandle case for this seems to be unavoidable.

Thanks, I will keep working base on the new type "ExternalImageType::TextureRectHandle".

The ps_image_rect and ps_image shader will be merged together. And I will use a feature "#ifdef" to switch the different functionality(texture2D and textureRect).

bors-servo · 2017-03-23T20:49:09Z

☔ The latest upstream changes (presumably #1003) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2017-03-24T21:26:52Z

☔ The latest upstream changes (presumably #966) made this pull request unmergeable. Please resolve the merge conflicts.

JerryShih · 2017-03-27T05:06:58Z

webrender/src/texture_cache.rs

+                                panic!("External texture handle should not go through texture_cache.");
+                            }
+                            ExternalImageType::ExternalBuffer => {
+                                let update_op = TextureUpdate {


This block comes from:
b4e91e1#diff-d52ac938b0c681e0d7bb3f946e47489fL825

JerryShih · 2017-03-27T05:07:32Z

webrender/src/texture_cache.rs

+                                panic!("External texture handle should not go through texture_cache.");
+                            }
+                            ExternalImageType::ExternalBuffer => {
+                                let update_op = TextureUpdate {


This block comes from:
b4e91e1#diff-d52ac938b0c681e0d7bb3f946e47489fL806

JerryShih · 2017-03-27T05:09:56Z

webrender/src/resource_cache.rs

@@ -651,6 +674,87 @@ impl ResourceCache {
            }
        }
    }
+    fn update_texture_cache(cached_images: &mut ResourceClassCache<ImageRequest, CachedImageInfo>,


This function is extracted from:
b4e91e1#diff-77cbdf7ba9ebae81feb38a64c21b8454L669
Since there are some "self" mutable borrow problem, this update_texture_cache() becomes static function.

nit: may look better if it's not static
What borrow problems did you encounter? Maybe we can work around them in a cleaner way.

JerryShih · 2017-03-27T05:11:13Z

webrender/src/resource_cache.rs

-                            texture_cache_id: image_id,
-                            epoch: image_template.epoch,
-                        });
+                    ExternalImageType::ExternalBuffer => {


We only call update_texture_cache for ExternalBuffer, Raw and Blob image.

JerryShih · 2017-03-27T05:18:05Z

webrender/src/resource_cache.rs

-            ImageData::ExternalHandle(id) => Some(id),
-            // raw and externalBuffer are all use resource_cache.
-            ImageData::Raw(..) | ImageData::ExternalBuffer(..) | ImageData::Blob(..) => None,
+        let external_image = match image_template.data {


Texture2DHandle, TextureRectHandle => some(ext_image)
Raw, Blob, ExternalBuffer => None

JerryShih · 2017-03-27T05:23:45Z

@kvark @nical @glennw
The image and image_shader are merged together now. Then, we could set the TEXTURE_RECT_FEATURE flag to work for TEXTURE_RECT type.

let ps_image_rect = try!{
    PrimitiveShader::new("ps_image",
                         &mut device,
                         &[ TEXTURE_RECT_FEATURE ],
                         options.precache_shaders)
};

Could you please take a look at this?

kvark

Found no major issues, but got a few things to address preferably.

kvark · 2017-03-27T17:07:27Z

webrender/res/ps_image.fs.glsl

    vec2 st = vTextureOffset + ((position_in_tile / vStretchSize) * vTextureSize);
    st = clamp(st, vStRect.xy, vStRect.zw);

    alpha = alpha * float(all(bvec2(step(position_in_tile, vStretchSize))));

+#ifndef WR_FEATURE_TEXTURE_RECT


I'd prefer having ifdef path instead of ifndef

kvark · 2017-03-27T17:07:56Z

webrender/res/ps_image.glsl

@@ -2,10 +2,13 @@
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

+// If this is in WR_FEATURE_TEXTURE_RECT mode, the rect and size use non-normalized
+// texture coordinates. Otherwise, it's use normalized texture coordinates. Please


fix it's use -> it uses

kvark · 2017-03-27T17:09:16Z

webrender/res/ps_image.vs.glsl

    // vUv will contain how many times this image has wrapped around the image size.
+#ifndef WR_FEATURE_TEXTURE_RECT
    vec2 texture_size = vec2(textureSize(sColor0, 0));


could be less of a code change if just texture_size is branched out, and the rest of the code is uniform

kvark · 2017-03-27T17:11:29Z

webrender/src/renderer.rs

+                    ExternalImageType::Texture2DHandle => TextureTarget::Default,
+                    ExternalImageType::TextureRectHandle => TextureTarget::Rect,
+                    _ => {
+                        panic!("Not a suitable image type.");


nit: actually include the type in the message

kvark · 2017-03-27T17:12:19Z

webrender/src/resource_cache.rs

@@ -651,6 +674,87 @@ impl ResourceCache {
            }
        }
    }
+    fn update_texture_cache(cached_images: &mut ResourceClassCache<ImageRequest, CachedImageInfo>,


nit: add new line before the function

kvark · 2017-03-27T17:13:31Z

webrender/src/resource_cache.rs

@@ -651,6 +674,87 @@ impl ResourceCache {
            }
        }
    }
+    fn update_texture_cache(cached_images: &mut ResourceClassCache<ImageRequest, CachedImageInfo>,


nit: may look better if it's not static
What borrow problems did you encounter? Maybe we can work around them in a cleaner way.

JerryShih · 2017-03-28T03:51:21Z

Review status: 0 of 14 files reviewed at latest revision, 9 unresolved discussions.

webrender/res/ps_image.fs.glsl, line 32 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I'd prefer having ifdef path instead of ifndef

Done.

webrender/res/ps_image.glsl, line 6 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

fix it's use -> it uses

Done.

webrender/res/ps_image.vs.glsl, line 37 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

could be less of a code change if just texture_size is branched out, and the rest of the code is uniform

Done.

webrender/src/renderer.rs, line 1673 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: actually include the type in the message

Done.

webrender/src/resource_cache.rs, line 677 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: add new line before the function

Done.

Comments from Reviewable

JerryShih · 2017-03-28T04:03:04Z

fn update_texture_cache2(&mut self) {
    println!("test");
}

fn finalize_image_request(&mut self,
                          request: ImageRequest,
                          image_data: Option<ImageData>,
                          texture_cache_profile: &mut TextureCacheProfileCounters) {
    let image_template = self.image_templates.get_mut(&request.key).unwrap();
    let image_data = image_data.unwrap_or_else(||{
        image_template.data.clone()
    });

    match image_template.data {
        ImageData::External(ext_image) => {
            match ext_image.image_type {
                ExternalImageType::Texture2DHandle |
                ExternalImageType::TextureRectHandle => {
                    // external handle doesn't need to update the texture_cache.
                }
                ExternalImageType::ExternalBuffer => {
                    self.update_texture_cache2();   // use member function

                    ResourceCache::update_texture_cache(&mut self.cached_images,
                                                        &mut self.texture_cache,
                                                        self.current_frame_id,
                                                        &request,
                                                        image_template,
                                                        image_data,
                                                        texture_cache_profile);
                }
            }
        }
        ImageData::Raw(..) | ImageData::Blob(..) => {
            ResourceCache::update_texture_cache(&mut self.cached_images,
                                                &mut self.texture_cache,
                                                self.current_frame_id,
                                                &request,
                                                image_template,
                                                image_data,
                                                texture_cache_profile);
        }
    }
}

The error message:

error[E0499]: cannot borrow `*self` as mutable more than once at a time
   --> src/resource_cache.rs:781:25
    |
768 |         let image_template = self.image_templates.get_mut(&request.key).unwrap();
    |                              -------------------- first mutable borrow occurs here
...
781 |                         self.update_texture_cache2();
    |                         ^^^^ second mutable borrow occurs here
...
803 |     }
    |     - first borrow ends here

Review status: 0 of 14 files reviewed at latest revision, 9 unresolved discussions.

Comments from Reviewable

bors-servo · 2017-03-28T19:07:38Z

☔ The latest upstream changes (presumably #1009) made this pull request unmergeable. Please resolve the merge conflicts.

kvark · 2017-03-28T19:56:11Z

@JerryShih
We can work around it by doing:

let (old_data, new_data) = {
    let image_template = self.image_templates.get_mut(&request.key).unwrap();
    let new_data = image_data.unwrap_or_else(||{
        image_template.data.clone()
    });
    (image_template.data.clone(), new_data)
};

match old_data {
...
}

This would contain the borrow of self.image_templates within a small scope and allow borrowing self for method calls inside the match arms.

JerryShih · 2017-03-29T04:58:05Z

@kvark

let (old_data, new_data) = {
    let image_template = self.image_templates.get_mut(&request.key).unwrap();
    let new_data = image_data.unwrap_or_else(||{
        image_template.data.clone()
    });
    (image_template.data.clone(), new_data)
};

match old_data {
...
}

Sorry, my test code doesn't show all detail. We should not use the cloned image_template here. It will be updated in update_texture_cache().

JerryShih · 2017-03-29T05:04:57Z

webrender/src/resource_cache.rs

-    fn update_texture_cache(cached_images: &mut ResourceClassCache<ImageRequest, CachedImageInfo>,
-                            texture_cache: &mut TextureCache,
-                            current_frame_id: FrameId,
+    fn update_texture_cache(&mut self,


Here is another version of update_texture_cache(). This version spends one more "image_templates" hash table lookup, but it turns to a member function.
I still prefer the static function one.

JerryShih · 2017-03-29T05:07:22Z

r? @kvark
5da747c

bors-servo · 2017-03-29T05:28:58Z

☔ The latest upstream changes (presumably #1004) made this pull request unmergeable. Please resolve the merge conflicts.

JerryShih · 2017-03-29T08:23:46Z

Sorry, rebase again.

kvark · 2017-03-29T19:26:22Z

Thanks @JerryShih !
Do you mind squashing the commits a little before merging?

JerryShih · 2017-03-30T02:47:37Z

@kvark
I don't know why the comments in reviewable are not shown in github. The concern of my last version is the one additional hash table lookup.

Here is another version of update_texture_cache(). This version spends one more "image_templates" hash table lookup, but it turns to a member function.
I still prefer the static function one.

…e handle type.

…xtureRectHandle.

…mage TextureRectHandle.

JerryShih · 2017-03-30T05:56:27Z

@kvark
If ths is not a problem, here are the squashed patches.

I don't know why the comments in reviewable are not shown in github. The concern of my last version is the one additional hash table lookup.
Here is another version of update_texture_cache(). This version spends one more "image_templates" hash table lookup, but it turns to a member function.
I still prefer the static function one.

kvark · 2017-03-30T13:14:36Z

@JerryShih I see, thanks. The double lookup should be fine (until we get deeper into the optimization stage), given that the relevant hashmap cell is already in the cache by that time.
@bors-servo r+

bors-servo · 2017-03-30T13:14:37Z

📌 Commit bbfefcc has been approved by kvark

bors-servo · 2017-03-30T13:16:44Z

⌛ Testing commit bbfefcc with merge 9800670...

@glennw

GL_TEXTURE_RECTANGLE texture target support @glennw @kvark @nical @sotaroikeda The hardware video decoder in mac platform is usually decoding the video content into MacIOSurface. The MacIOSurface could just map to an GL_TEXTURE_RECTANGLE gl texture without any uploading. That's good for video playing. These patches try to add a new type of external gl rect image and its corresponding shader.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/997)

bors-servo · 2017-03-30T13:44:27Z

☀️ Test successful - status-travis
Approved by: kvark
Pushing 9800670 to master...

JerryShih force-pushed the gl-texture-rect-support branch from 92be058 to d0ffcd4 Compare March 21, 2017 05:55

JerryShih commented Mar 21, 2017

View reviewed changes

JerryShih force-pushed the gl-texture-rect-support branch from bf6b689 to f06cbae Compare March 24, 2017 09:05

JerryShih force-pushed the gl-texture-rect-support branch from f06cbae to 6b57949 Compare March 27, 2017 04:12

JerryShih commented Mar 27, 2017

View reviewed changes

kvark reviewed Mar 27, 2017

View reviewed changes

JerryShih force-pushed the gl-texture-rect-support branch from 6b57949 to 3a3f830 Compare March 28, 2017 04:10

JerryShih force-pushed the gl-texture-rect-support branch from 3a3f830 to 5da747c Compare March 29, 2017 05:00

JerryShih commented Mar 29, 2017

View reviewed changes

JerryShih force-pushed the gl-texture-rect-support branch from 5da747c to 4c78f6f Compare March 29, 2017 08:22

kvark approved these changes Mar 29, 2017

View reviewed changes

JerryShih added 4 commits March 30, 2017 13:49

Use struct ExternalImageData to present the different external textur…

86b00a7

…e handle type.

Add new TextureTarget::Rect texture target type for external image Te…

1305f38

…xtureRectHandle.

Add a new TEXTURE_RECT_FEATURE feature in image shader for external i…

dec3102

…mage TextureRectHandle.

Update ImageData type in wrench.

bbfefcc

JerryShih force-pushed the gl-texture-rect-support branch from 4c78f6f to bbfefcc Compare March 30, 2017 05:54

bors-servo merged commit bbfefcc into servo:master Mar 30, 2017

This was referenced Mar 30, 2017

Tile external buffer images when they exceed the maximum texture size. #1036

Merged

Favor the function over the method syntax when cloning a reference counted pointer. #1037

Merged

Expose images to the BlobImageRenderer. #1018

Merged

kvark mentioned this pull request Mar 31, 2017

Support shared IOSurface of GL_TEXTURE_RECTANGLE type #642

Closed


		alpha = alpha * float(all(bvec2(step(position_in_tile, vStretchSize))));

		oFragColor = vec4(alpha) * texture(sColor0, st);

GL_TEXTURE_RECTANGLE texture target support #997

GL_TEXTURE_RECTANGLE texture target support #997

Conversation

JerryShih commented Mar 21, 2017 • edited by larsbergstrom Loading

JerryShih commented Mar 21, 2017

bors-servo commented Mar 21, 2017

JerryShih left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JerryShih Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nical commented Mar 21, 2017

kvark commented Mar 21, 2017

nical commented Mar 21, 2017

kvark commented Mar 21, 2017

nical commented Mar 21, 2017

JerryShih commented Mar 22, 2017

kvark commented Mar 22, 2017 • edited Loading

JerryShih commented Mar 22, 2017

bors-servo commented Mar 23, 2017

bors-servo commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JerryShih Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JerryShih commented Mar 27, 2017

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JerryShih commented Mar 28, 2017

JerryShih commented Mar 28, 2017

bors-servo commented Mar 28, 2017

kvark commented Mar 28, 2017

JerryShih commented Mar 29, 2017

Choose a reason for hiding this comment

JerryShih commented Mar 29, 2017

bors-servo commented Mar 29, 2017

JerryShih commented Mar 29, 2017

kvark commented Mar 29, 2017

JerryShih commented Mar 30, 2017

JerryShih commented Mar 30, 2017

kvark commented Mar 30, 2017

bors-servo commented Mar 30, 2017

bors-servo commented Mar 30, 2017

bors-servo commented Mar 30, 2017

JerryShih commented Mar 21, 2017 •

edited by larsbergstrom

Loading

JerryShih left a comment •

edited

Loading

JerryShih Mar 21, 2017 •

edited

Loading

kvark commented Mar 22, 2017 •

edited

Loading

JerryShih Mar 27, 2017 •

edited

Loading